Add preliminary support for json metadata client discovery #268
Conversation
|
@pfefferle Can you have a look? |
|
Ha, missed this PR the other day. I'll see if I can try it tonight! |
|
Feedback is appreciated |
| libxml_use_internal_errors( true ); | ||
| if ( function_exists( 'mb_convert_encoding' ) ) { | ||
| $content = mb_convert_encoding( $content, 'HTML-ENTITIES', mb_detect_encoding( $content ) ); | ||
| $content_type = wp_remote_retrieve_header( $response, 'content-type' ); |
There was a problem hiding this comment.
parse() seems like it should be designated as static since that's how it is used. Also it seems as though it could get to the bottom and not return so I think it needs a return at the bottom. I also suggest a doc block describe the params and what it does if there's an error.
| $content = mb_convert_encoding( $content, 'HTML-ENTITIES', mb_detect_encoding( $content ) ); | ||
| $content_type = wp_remote_retrieve_header( $response, 'content-type' ); | ||
| if ( 'application/json' === $content_type ) { | ||
| $json = json_decode( wp_remote_retrieve_body( $response ), true ); |
There was a problem hiding this comment.
| $json = json_decode( wp_remote_retrieve_body( $response ), true ); | |
| /** @var $json array{ | |
| * client_id: string, | |
| * client_name: string, | |
| * logo_uri: string | |
| * } | |
| */ | |
| $json = json_decode( wp_remote_retrieve_body( $response ), true ); |
There was a problem hiding this comment.
I like using comments to describe what we expect to be in json like this since it makes identifying and using them slightly easier.
|
So, the PR works! Also (I can't add a comment because it isn't new), this here:
Also, the issue I had earlier was caused because of: If the response contains no Not sure what happens if no meaningful client details are discovered? I liked Aaron's suggestion (I think) of just showing a URL ...? |
#267 per request, basic support for the client ID being a JSON file using the properties identified.
@aaronpk This should work based on the notes.
If this gets adopted into the spec, I'll probably cut the unsupported manifest side file code, as that proposal never took off, and simplify for JSON with a HTML MF2 fallback.